Support Platform-specify wheels and add rpds_py as a dependency #41143
Conversation
- Introduced a new `tarballs_info` property in the Package class to manage multiple tarballs for platform-specific wheels. - Implemented `find_tarball_for_platform` method to select the appropriate tarball based on the current platform and Python version. - Enhanced the `Tarball` class to handle platform-specific wheels, including methods for downloading wheels using pip and checking for cached wheels. - Updated checksum verification to support multiple tarballs, ensuring integrity checks for downloaded files. - Added logic to fall back to traditional download methods if pip fails or if no cached wheels are found. - Created new dependencies and package version files for the `rpds_py` package.
Remove unused import 'unicode_to_str' from formatter.py
|
Documentation preview for this PR (built with commit 7d67167; changes) is ready! 🎉 |
…h Python version and free-threading status from Sage's Python, and remove pip download logic for platform-specific wheels.
…mproved compatibility checks and streamline the process of finding the best matching tarball for the current platform and Python version.
…r improved functionality
…aging.utils, improving compatibility with multi-platform wheels.
Updated the download fallback method in tarball.py.
Removed 'text=True' argument from subprocess calls.
cxzhong
changed the title
cxzhong
changed the title
cxzhong
changed the title
Removed steps for making distribution and downloading packages with '--disable-download-from-upstream-url'.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for platform-specific wheels in the Sage build system and introduces rpds_py as a new dependency. The implementation allows packages to specify multiple platform-specific wheel files in their checksums.ini, with automatic selection based on the target platform's compatibility tags.
Key Changes:
- Extended tarball and package handling to support multiple platform-specific wheels per package
- Added platform detection using
packaging.tagsandpackaging.utilsmodules via subprocess calls - Added rpds_py package with 52 platform-specific wheel entries for Python 3.11-3.14 across multiple platforms
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| build/sage_bootstrap/tarball.py | Extended Tarball class to handle multi-tarball packages with new methods _download_multiple_wheels() and _download_single_wheel() for platform-specific wheel downloading |
| build/sage_bootstrap/package.py | Added tarballs_info property and find_tarball_for_platform() method for platform detection; modified _init_checksum() to parse multiple tarball entries |
| build/pkgs/rpds_py/type | New package type file marking rpds_py as a "standard" package |
| build/pkgs/rpds_py/package-version.txt | Version specification for rpds_py 0.28.0 |
| build/pkgs/rpds_py/dependencies | Dependencies declaration for rpds_py requiring pip and packaging |
| build/pkgs/rpds_py/checksums.ini | Comprehensive checksums for 52 platform-specific wheels covering Python 3.11-3.14 across Linux, macOS, and Windows |
| build/pkgs/rpds_py/SPKG.rst | Documentation for the new rpds_py package describing its purpose and platform support |
| build/pkgs/jsonschema/dependencies | Updated to add rpds_py as a dependency for jsonschema |
| build/make/Makefile.in | Added SAGE_DOWNLOAD_ALL_WHEELS environment variable for downloading all wheels during dist creation |
| .github/workflows/release.yml | Removed duplicate steps and simplified workflow (though this introduced conditional logic issues) |
Comments suppressed due to low confidence (2)
build/sage_bootstrap/tarball.py:19
- Import of 'sys' is not used.
import sys
build/sage_bootstrap/tarball.py:20
- Import of 'subprocess' is not used.
import subprocess
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, tarball in enumerate(tarballs): | ||
| info = { | ||
| 'tarball': tarball, | ||
| 'sha256': sha256s[i] if i < len(sha256s) else None, | ||
| 'sha1': sha1s[i] if i < len(sha1s) else None, | ||
| 'upstream_url': upstream_urls[i] if i < len(upstream_urls) else None, | ||
| } | ||
| self.__tarballs_info.append(info) |
There was a problem hiding this comment.
Potential bug: The code assumes that checksums.ini entries are ordered such that the i-th sha256 corresponds to the i-th tarball. However, if a line in checksums.ini has only a tarball without a corresponding sha256 (or vice versa), the indices will be misaligned. For example:
tarball=file1.whl
tarball=file2.whl
sha256=abc123
Would incorrectly assign sha256=abc123 to tarball=file1.whl (index 0) instead of file2.whl. Consider grouping entries by blank lines or using a state machine to ensure correct pairing of tarball/sha256/sha1/upstream_url entries.
build/sage_bootstrap/package.py
Outdated
| python_code = 'import packaging.utils, json, sys; _, _, _, tags = packaging.utils.parse_wheel_filename(sys.argv[1]); print(json.dumps([str(t) for t in tags]))' | ||
| result = subprocess.run( | ||
| [sage_script, '-python', '-c', python_code, tarball], |
There was a problem hiding this comment.
[nitpick] Potential security consideration: The code executes subprocess.run() with user-controlled input (tarball variable from checksums.ini). While the tarball filename comes from a trusted source (the package's checksums.ini file), it's passed directly to a subprocess call. Consider validating the tarball filename to ensure it doesn't contain shell metacharacters or path traversal sequences, even though it's from a trusted source.
| except Exception as e: | ||
| log.warning(f'Error checking cached file: {e}') |
There was a problem hiding this comment.
The broad except Exception handler at line 306 catches all exceptions, including potentially serious ones like KeyboardInterrupt (though technically it's BaseException). It would be better to catch specific exceptions like (IOError, ValueError, ChecksumError) to avoid masking unexpected errors. The same issue exists at line 358.
Co-authored-by: Copilot <[email protected]>
|
TODO: I will delete python 3.11 packages and 3.13t packages |
|
I think this is the right approach, but I happen to be on a platform (riscv/musl) where binary wheels don't exist. One thing I noticed though is that the new package can be disabled when |
yes, I will fix this. I think I can report to upstream to make wheel for some specify platform. Otherwise you have to have a rust toolchain. |
Do you think this way is better or we just remove the notebook dependencies from sage-distro is better?( But some notebook dependencies are also dependencies for building documents, like juypter-sphinx) |
I have done it. |
2 participants
This PR is to allow the platform-specify wheels and preserve the backward compatibility. Then add a package rpds_py as a dependency of jsonschema. and I set when running
make distto make a distribution. it will download all wheels.📝 Checklist
⌛ Dependencies